_{Comment @coderabbitai help to get the list of available commands.}

Review

This PR adds test_start_task_execute_runs_agent_to_terminal, the first API-level test to exercise /tasks/{id}/start?execute=true — closing the gap where the agent-dispatch branch (_run_agent → runtime.execute_agent) had zero coverage. It's a single-file, test-only change with no production code touched, so risk is low. I traced the MockProvider path (get_provider("mock") → text-only "Mock response" → react_agent.py:518 not response.has_tool_calls → classify_error_for_blocker returns None for that text → completes), the /start router's fire-and-forget thread (tasks_v2.py:694-730), and the thread-diff/join pattern used to avoid the teardown race — all of that checks out and the reasoning in the PR description matches the actual code.

One finding worth a look:

tests/ui/test_v2_routers_integration.py:773 — the "produced output" assertion doesn't test what the PR description claims it tests.
RunOutputLogger.__init__ (codeframe/core/streaming.py:87-103) opens the run's output log file in append mode (open(self.log_path, "a", ...)) unconditionally at construction — before any agent iteration runs. run_output_exists() is just path.exists(). Since execute_agent constructs RunOutputLogger near the very start of the call (runtime.py:693), the file exists from that point on regardless of whether any output was actually written. So assert streaming.run_output_exists(workspace, run_id) is True would pass even in a hypothetical where zero events got emitted (e.g., execute_agent errored out immediately after constructing the logger, before writing anything) — it isn't "evidence the BUILD path actually emitted events" as the PR description states, just evidence the run started. Combined with the status == COMPLETED assertion above it, the test still provides reasonable overall signal, but if the intent is specifically to prove events were emitted, consider asserting on the file's content/size (e.g., get_run_output_path(...).stat().st_size > 0, or checking for an expected marker) instead of mere existence.

Nothing else stood out — the thread-snapshot/join approach to avoid the teardown DB race is sound (starlette's TestClient, used here without a with block, opens and fully tears down its own anyio portal thread synchronously per-request, so the diff only ever catches the intended _run_agent daemon thread), the env var injection via monkeypatch.setenv is read inside the joined thread before monkeypatch's teardown reverts it, and the module's existing pytestmark = pytest.mark.v2 already covers the new test per CLAUDE.md convention.

🤖 Generated with Claude Code

Thanks — good catch on the run_output_exists finding. Confirmed: RunOutputLogger.__init__ opens the output file in append mode at execute_agent start (streaming.py:103, before any iteration), and run_output_exists() is just path.exists() — so the old assertion only proved the run started. Fixed in 4ca329b by asserting get_run_output_path(...).stat().st_size > 0, which proves the BUILD path actually wrote output. Verified the completed mock run produces a non-empty file.

Review

Re-reviewed after the fix in 4ca329b. The PR adds test_start_task_execute_runs_agent_to_terminal, the first API-level test to exercise /tasks/{id}/start?execute=true (the _run_agent → runtime.execute_agent BUILD-path branch), which previously had zero coverage. Still a single-file, test-only change — no production code touched.

Previously flagged issue — confirmed fixed. My earlier review noted that streaming.run_output_exists() is just path.exists(), and RunOutputLogger.__init__ (codeframe/core/streaming.py:87-103) opens the log file in append mode unconditionally at execute_agent start (runtime.py:693), so the old assertion only proved the run started, not that it emitted output. The current diff (tests/ui/test_v2_routers_integration.py:773) now asserts output_path.stat().st_size > 0, which does require content to have actually been written and flushed — RunOutputLogger.write() flushes after every write (streaming.py:114), and execute_agent doesn't spawn any further threads (confirmed via grep on runtime.py), so joining the single _run_agent daemon thread before this assertion is sufficient to guarantee the writes landed. This resolves the gap.

Nothing new found. I re-traced the rest of the test against the current code:

• start_single_task (tasks_v2.py:643) is async def, and thread.start() for _run_agent happens synchronously inside the coroutine before the response is returned (tasks_v2.py:729-730), so the threads_before snapshot taken right before the POST reliably captures only threads that exist prior to dispatch — the fire-and-forget worker is guaranteed to show up in the post-request threading.enumerate() diff.
• The test_client/test_workspace fixtures (test_v2_routers_integration.py:25-68) do shutil.rmtree(temp_dir, ignore_errors=True) at teardown with no synchronization otherwise, so the join-before-teardown pattern here is in fact necessary, not defensive-programming.
• monkeypatch.setenv("CODEFRAME_LLM_PROVIDER", "mock") is read by get_provider("mock") inside the joined thread, before the function-scoped monkeypatch fixture reverts it — ordering is safe.

No new correctness, security, or coverage issues stood out in this pass. The 30s poll deadline / 10s join timeout are reasonable for CI and, per the PR description, the test has been run repeatedly without flaking.

🤖 Generated with Claude Code